Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use FontAwesome for navigation menu #2959

Closed
wants to merge 2 commits into from

Conversation

rhwood
Copy link
Contributor

@rhwood rhwood commented May 16, 2021

This is an enhancement or feature.

Summary

Use a FontAwesome icon for the navigation menu displayed when navigation items exceed the available width. Combined with #2774, this theme should be using FontAwesome characters for all built-in iconography.

Context

This is not related to any open issue.

@mmistakes
Copy link
Owner

mmistakes commented May 17, 2021

Only negative I see with moving to Font Awesome is the icon transition isn't great. There's no smooth animation from the bars icon to the close icon... it's now super janky as there's no way to ease the two icons.

I think in this case I'd rather stick with the pure CSS menu icon instead of using Font Awesome's set.

@iBug
Copy link
Collaborator

iBug commented May 17, 2021

I'm with @mmistakes. While FA is great, the menu button is at a mature state that opting in for FA brings little advantage over the status quo.

@rhwood
Copy link
Contributor Author

rhwood commented May 17, 2021

I was thinking more about replaceability of the icon(s) without essentially having to override a significant amount of CSS and/or javascript. By only having to override to two variables to change the icon, I can replace the hamburger icon with the kebab (as used in android and other UIs)

I should also note that I only asked #2957 while working on this--until I dug into the CSS while working on this, I hadn't known the X transform was supposed to be there as I never saw it.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

This pull request will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

@mmistakes
Copy link
Owner

Closing for the reasons cited above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants